Simple Agent V2 #5501
Simple Agent V2 #5501
Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
7 Jobs Failed: Run Integration Tests v2 / prepare-build failed on "Generate OpenAPI schema"Run Integration Tests v2 / required failed on "Run actions/github-script@v7"5 jobs failed running on non-Blacksmith runners. Summary: 4 successful workflows, 5 failed workflows
Last updated: 2025-09-26 23:28:53 UTC |
|
|
||
| if message_id is None: | ||
| raise ValueError("Message ID is required") | ||
| if packet != {"type": "event"}: |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 886-887
if packet != {"type": "event"}:
print(packet)This print statement is debug code and should not be present in production. It should be removed or replaced with a proper logging call.
| message_id=message_id, | ||
| error_msg=error_msg, | ||
| top_documents=top_documents, | ||
| cited_documents={}, |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 902-905
cited_documents={},
message_id=0,
error_msg=None,
top_documents=[],These hardcoded default values indicate a significant loss of functionality. The gather_stream function no longer extracts citations, message ID, error messages, or top documents from the stream, which were previously handled. This is a critical functional regression.
| @@ -0,0 +1 @@ | |||
| # Turn module for chat functionality | |||
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 1-1
# Turn module for chat functionalityThis comment could be replaced by a package docstring for better documentation practices and accessibility. While not strictly problematic, it's a missed opportunity for more robust documentation.
| @@ -0,0 +1 @@ | |||
| # Infrastructure module for chat turn orchestration | |||
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 1-1
# Infrastructure module for chat turn orchestrationUsing a # comment for module-level documentation is less standard than using a module docstring. Docstrings are accessible via help() and are used by documentation generation tools, improving maintainability and discoverability.
| ) | ||
| elif packet_type == "response.output_item.done": | ||
| return SectionEnd(type="section_end") | ||
| packet_class(**filtered_data) |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 157-158
packet_class(**filtered_data)
return packet_class(**filtered_data)The line packet_class(**filtered_data) creates an instance of packet_class but immediately discards it, as the very next line creates another identical instance which is then returned. This is redundant and inefficient.
| emitter = Emitter(bus) | ||
| current_context = contextvars.copy_context() | ||
| dependencies.emitter = emitter | ||
| t = threading.Thread( |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 39-56
t = threading.Thread(
target=current_context.run,
args=(
turn_func,
messages,
dependencies,
),
daemon=True,
)
t.start()
while True:
pkt: Packet = emitter.bus.get()
print("packet", pkt)
if pkt.obj == OverallStop(type="stop"):
yield pkt
break
else:
yield pktThis code block contains several issues: a debugging print statement, a potential bug in the OverallStop comparison that could lead to an infinite loop, and a critical reliability flaw where unhandled exceptions in the turn_func thread will cause the emitter.bus.get() call to block indefinitely, hanging the generator. Additionally, the while loop's if/else structure can be simplified for better readability.
|
|
||
| def default_packet_translation(ev: object) -> PacketObj | None: | ||
| if isinstance(ev, RawResponsesStreamEvent): | ||
| # TODO: might need some variation here for different types of models |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 11-11
# TODO: might need some variation here for different types of modelsThe TODO comment indicates that the current implementation is incomplete or might require future adjustments for different model types. This suggests a known limitation or an area that needs further development, impacting the long-term maintainability and completeness of the feature.
| # OpenAI packet translator | ||
| obj: PacketObj | None = None | ||
| if ev.data.type == "response.created": | ||
| obj = MessageStart(type="message_start", content="", final_documents=None) |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 15-15
obj = MessageStart(type="message_start", content="", final_documents=None)The content and final_documents fields are hardcoded to an empty string and None respectively for response.created events. While this might be the desired initial state, it assumes that response.created events never carry initial content or document references. If future RawResponsesStreamEvent data for response.created includes such information, this hardcoding would lead to data loss or incorrect message initialization.
| # TODO: might need some variation here for different types of models | ||
| # OpenAI packet translator | ||
| obj: PacketObj | None = None | ||
| if ev.data.type == "response.created": |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 14-19
if ev.data.type == "response.created":
obj = MessageStart(type="message_start", content="", final_documents=None)
elif ev.data.type == "response.output_text.delta":
obj = MessageDelta(type="message_delta", content=ev.data.delta)
elif ev.data.type == "response.output_item.done":
obj = SectionEnd(type="section_end")The function only handles three specific ev.data.type values. Any other RawResponsesStreamEvent type will cause obj to remain None (initialized on line 13) and the function will silently return None. This lack of explicit handling for unknown or future event types can lead to unexpected behavior or silent failures downstream without any indication of what was missed.
| data: list[dict[str, dict[str, str]]] | None = None, | ||
| remote_dataset_name: str | None = None, | ||
| provider: EvalProvider = get_default_provider(), | ||
| no_send_logs: bool = False, |
There was a problem hiding this comment.
⚠️ Code Issue
Lines: 103-103
no_send_logs: bool = False,Using negative phrasing for boolean parameters like no_send_logs can sometimes lead to confusion, especially when combined with negation in conditional logic. A positive name like send_logs would be more intuitive.
Description
[Provide a brief description of the changes in this PR]
How Has This Been Tested?
[Describe the tests you ran to verify your changes]
Additional Options
Summary by cubic
Introduces a new streaming chat turn pipeline (Simple Agent V2) and a deep-research scratchpad agent, and wires chat processing to the new runner. Adds Braintrust tracing controls and an eval CLI flag to disable log sending.
New Features
Refactors